Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move with_globals setup from run_compiler to run #53624

Merged
merged 2 commits into from
Aug 27, 2018
Merged

Move with_globals setup from run_compiler to run #53624

merged 2 commits into from
Aug 27, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Aug 23, 2018

An alternative to #53526

Note this breaks some miri stuff and clippy since they call run_compiler directly, and they now need to also call with_globals cc @rust-lang/dev-tools

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 23, 2018
@alexreg
Copy link
Contributor

alexreg commented Aug 23, 2018

Does this solve all the issues in #53420?

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 23, 2018

@alexreg No, it just makes it print the ICE as it normally should be printed instead of cannot access a scoped thread local variable without calling set first

@nrc
Copy link
Member

nrc commented Aug 23, 2018

This will break RLS and Rustfmt too, I think.

@earthengine
Copy link

earthengine commented Aug 23, 2018 •
edited
Loading

Although this passes the tests, but I am afraid that if the monitor itself captured an error and need to emit, we would still have the same problem. This is the consideration that makes me put the with_globals outside the monitor call.

I originally goes through this and in turn created run_compiler_impl as a private variant that don't do wraps, to avoid breakage of outside calls.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 23, 2018

does the monitor ever need to emit diagnostics?

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 23, 2018

The monitor does emit diagnostics, but it only uses dummy spans, which does not seem to access libsyntax globals. I've tested that the ICE printing still works locally.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 23, 2018

@nrc RLS uses run, so it doesn't need any changes. rustfmt doesn't seems to use the compiler at all, just libsyntax, so no changes should be needed.

@alexreg
Copy link
Contributor

alexreg commented Aug 23, 2018

@Zoxc Didn't the patch I sent to you work? And avoid breaking other things?

This was referenced Aug 26, 2018
Merged
Closed
@oli-obk
Copy link
Contributor

oli-obk commented Aug 26, 2018

I updated clippy. It should work with this PRs changes now. Can you update the clippy submodule to the current master? Then this PR doesn't break clippy at all

@alexreg
Copy link
Contributor

alexreg commented Aug 26, 2018

@oli-obk Good stuff. How are you getting on with the underlying MIR issue?

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
######################################################################    98.0%
######################################################################## 100.0%
[00:02:08] extracting /checkout/obj/build/cache/2018-08-01/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:02:13]     Updating registry `https://github.com/rust-lang/crates.io-index`
[00:02:36] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:36] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:36] Makefile:81: recipe for target 'prepare' failed
[00:02:36] make: *** [prepare] Error 1
[00:02:37] Command failed. Attempt 2/5:
[00:02:37] Command failed. Attempt 2/5:
[00:02:37] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:37] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:37] make: *** [prepare] Error 1
[00:02:37] Makefile:81: recipe for target 'prepare' failed
[00:02:39] Command failed. Attempt 3/5:
[00:02:39] Command failed. Attempt 3/5:
[00:02:39] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:39] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:39] make: *** [prepare] Error 1
[00:02:39] Makefile:81: recipe for target 'prepare' failed
[00:02:42] Command failed. Attempt 4/5:
[00:02:42] Command failed. Attempt 4/5:
[00:02:43] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:43] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:43] Makefile:81: recipe for target 'prepare' failed
[00:02:43] make: *** [prepare] Error 1
[00:02:47] Command failed. Attempt 5/5:
[00:02:47] Command failed. Attempt 5/5:
[00:02:47] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:47] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:47] make: *** [prepare] Error 1
[00:02:47] Makefile:81: recipe for target 'prepare' failed
[00:02:47] The command has failed after 5 attempts.
travis_time:end:14570ece:start=1535329687676662679,finish=1535329855435349045,duration=167758686366
---
travis_time:end:25ad7200:start=1535329855879001484,finish=1535329855892545901,duration=13544417
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:28f65ef0
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:01158c53
travis_time:start:01158c53
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:052459a5
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@earthengine
Copy link

earthengine commented Aug 27, 2018 •
edited
Loading

I am a bit concerned about this change as it modifies the public API for run_compiler. If we want to stabilize this call to the public, to enable third party tools development, we should not expect the user to call with_globals by themselves. Therefore, I believe it would be better to make sure run_compiler as a public interface being always wrapped, and use an internal function for the unwrapped calls.

This will also provide a less breaking changes to easily fix the current check fails.

@alexreg
Copy link
Contributor

alexreg commented Aug 27, 2018

Looks like CI is failing because of the submodule update? Or is it just transient?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 27, 2018

@bors r+

@earthengine If we ever want to stabilize this API we'd need to do so many other changes, this seems like the least of the worries I have with that.

We could rather check if we can merge run and run_compiler by looking at all use cases.

@bors
Copy link
Contributor

bors commented Aug 27, 2018

📌 Commit 0972658 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2018
@bors
Copy link
Contributor

bors commented Aug 27, 2018

⌛ Testing commit 0972658 with merge 291d958...

bors added a commit that referenced this pull request Aug 27, 2018
@bors
Auto merge of #53624 - Zoxc:ice-fix, r=oli-obk
291d958 
Move with_globals setup from run_compiler to run

An alternative to #53526

Note this breaks some miri stuff and clippy since they call `run_compiler` directly, and they now need to also call `with_globals ` cc @rust-lang/dev-tools

r? @oli-obk
@bors
Copy link
Contributor

bors commented Aug 27, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 291d958 to master...

@bors bors merged commit 0972658 into rust-lang:master Aug 27, 2018
@bors bors mentioned this pull request Aug 27, 2018
Merged
@Zoxc Zoxc deleted the ice-fix branch August 29, 2018 05:17
This was referenced Aug 31, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Zoxc @alexreg @nrc @earthengine @oli-obk @rust-highfive @bors